Experimental autodetection of 64 bit timespec on 32 bit platforms - #18
Experimental autodetection of 64 bit timespec on 32 bit platforms#18diwic wants to merge 2 commits into
Conversation
| impl timespec { | ||
| fn is64(&self) -> bool { self.0[2] != 0 || self.0[3] != 0 } | ||
| pub fn tv_sec(&self) -> i64 { self.0[0] as i64 } | ||
| pub fn tv_nsec(&self) -> i64 { if self.is64() { (self.0[2] + self.0[3]) as i64 } else { self.0[1] as i64 } } |
There was a problem hiding this comment.
self.0[2] + self.0[3] is a nice trick and works because either field is just padding. It's a trick though. For clarity how would you feel about gating on endianness?
There was a problem hiding this comment.
@roderickvd Having thought about it, I think we should apply this fix on little endian only. Big endian is incredibly uncommon.
There was a problem hiding this comment.
As uncommon as it may be, wouldn't it be nice for alsa-sys to work regardless?
|
|
||
| #[cfg(target_pointer_width = "32")] | ||
| #[derive(Copy, Clone, Eq, PartialEq, Default)] | ||
| #[repr(C)] |
There was a problem hiding this comment.
Should we also do #[repr(C, align(8))] to ensure it doesn't segfault when ALSA tries to write? Rust may not guarantee the same alignment.
There was a problem hiding this comment.
And good idea to use those native-width i64s for alignment 😄
| use libc::{FILE, pid_t, pollfd, timeval}; | ||
|
|
||
| #[cfg(target_pointer_width = "64")] | ||
| #[derive(Copy, Clone, Eq, PartialEq, Default)] |
There was a problem hiding this comment.
Debug may be nice to implement.
There was a problem hiding this comment.
In your rewrite I think these were dropped altogether (because they can't be derived on the union). That creates an asymmetry with libc::timespec, which does derive them.
| #[cfg(target_pointer_width = "32")] | ||
| impl timespec { | ||
| fn is64(&self) -> bool { self.0[2] != 0 || self.0[3] != 0 } | ||
| pub fn tv_sec(&self) -> i64 { self.0[0] as i64 } |
There was a problem hiding this comment.
If is64 we should retain the upper half:
pub fn tv_sec(&self) -> i64 {
if self.is64() {
#[cfg(target_endian = "little")]
((self.0[1] as i64) << 32) | (self.0[0] as u32 as i64)
#[cfg(target_endian = "big")]
((self.0[0] as i64) << 32) | (self.0[1] as u32 as i64)
} else {
self.0[0] as i64
}
}...I think 🤯 time problems are hard.
There was a problem hiding this comment.
@roderickvd Yeah and there is an edge case where is64 would return false because tv_nsec actually is 0. This would fail on big endian :-(
|
@roderickvd Based on your review I made another attempt. Let me know what you think about it. The way it's written I don't think it needs any cfg on target_endian. |
There was a problem hiding this comment.
Pull request overview
This PR attempts to make snd_htimestamp_t (timespec) work on 32-bit platforms regardless of whether the underlying ALSA/libc uses a 32-bit or 64-bit struct timespec, by introducing a custom timespec type with runtime “autodetection”.
Changes:
- Replaced the direct
libc::timespecimport with a crate-definedtimespectype. - Added
tv_sec()/tv_nsec()accessor methods and aDefaultimpl for the newtimespec.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[derive(Copy, Clone)] | ||
| #[repr(C)] | ||
| pub union timespec { | ||
| t: libc::timespec, | ||
| t64: [i64; 2], | ||
| } | ||
|
|
||
| #[cfg(not(target_pointer_width = "32"))] | ||
| impl timespec { | ||
| #[inline(always)] | ||
| pub fn tv_sec(&self) -> i64 { unsafe { self.t.tv_sec as i64 } } | ||
| #[inline(always)] | ||
| pub fn tv_nsec(&self) -> i64 { unsafe { self.t.tv_nsec as i64 } } | ||
| } | ||
|
|
||
| #[cfg(target_pointer_width = "32")] | ||
| impl timespec { | ||
| unsafe fn is64(&self) -> bool { self.t64[1] != 0 } | ||
| pub fn tv_sec(&self) -> i64 { unsafe { if self.is64() { self.t64[0] } else { self.t.tv_sec as i64 } } } | ||
| pub fn tv_nsec(&self) -> i64 { unsafe { if self.is64() { self.t64[1] } else { self.t.tv_nsec as i64 } } } | ||
| } | ||
|
|
||
| impl Default for timespec { | ||
| fn default() -> Self { unsafe { zeroed() } } | ||
| } |
There was a problem hiding this comment.
The LLM has got a point, but the proposed fix would just recreate the segfault that we're trying to address.
roderickvd
left a comment
roderickvd
left a comment
There was a problem hiding this comment.
The union is a great find, would never have thought of that. It doesn't solve the problem of misdetection on big-endian 32 bit though, and Copilot has got a point w.r.t. to the other methods.
As an alternative I submitted #19 with a build probe that should have none of these side-effects, what do you think?
Exploits the fact (?) that even though the fields are 64 bit, only 32 of the bits are used in practice: